[AgentX] Update vLLM DeepSeek-V4 B300 aggregate / 更新 vLLM DeepSeek-V4 B300 聚合配置#2233
[AgentX] Update vLLM DeepSeek-V4 B300 aggregate / 更新 vLLM DeepSeek-V4 B300 聚合配置#2233majunze2001 wants to merge 8 commits into
Conversation
Carry the B300-only launcher, config search space, and changelog scope from PR SemiAnalysisAI#2202. 中文：拆分 DeepSeek-V4 B300 vLLM AgentX 配方，仅保留 PR SemiAnalysisAI#2202 中的 B300 启动器、配置搜索空间和变更日志范围。
Point the split B300 changelog entry to PR SemiAnalysisAI#2225. 中文：将拆分后的 B300 变更日志条目链接到 PR SemiAnalysisAI#2225。
Signed-off-by: Jeff Ma <jeffjma@umich.edu>
Signed-off-by: Jeff Ma <jeffjma@umich.edu>
Signed-off-by: Jeff Ma <jeffjma@umich.edu>
majunze2001
changed the title
majunze2001
requested review from
Ankur-singh,
jgangani and
kedarpotdar-nv
as code owners
ivanium
added
full-sweep-enabled
NVIDIA
agentx
|
@claude review |
|
Claude finished @ivanium's task in 4m 25s —— View job Code Review
This PR restructures the DSV4 FP4 B300 vLLM agentic sweep: it converts DEP8 to a GPU-resident arm with its own tuning ( What I verified ✅
Issues1. The changelog line no longer matches what
Benchmarks trigger off 2. Stale comments in
3. The edited changelog entry keeps Observations (no action needed)
Overall: script changes look correct and well-commented; please sync the changelog description (and ideally the script header) with the final arm/conc structure before merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
perf-changelog.yaml:4794-4798— This diff edits the description of the existing PR #2225 entry in perf-changelog.yaml in place instead of appending a new entry for PR #2233, which violates AGENTS.md's append-only rule and will fail CI (bothprocess_changelog.py's deletion check andvalidate_perf_changelog.py's immutable-entry check, since the pr-link stays at 2225 while the description changes). The rewritten text is also inaccurate: it claims a combined 'TP4/TP8' GPU-resident arm (only TP4 exists now), the wrong DEP8 conc-list ([64,96,112,128,144,160,176,192,224], not[32,64,96,128,160,192,196,224,228]), the wrong DEP4 conc-list ([32,40,48,56,64,72], not[8,16,24,32,40,48,56,64,72]), and a stale reference to Mooncake which this PR removes for DEP4.Extended reasoning...
What's wrong. The diff hunk
@@ -4794,5 +4794,5 @@inperf-changelog.yamlrewrites the description bullet of an existing entry (config-keys: dsv4-fp4-b300-vllm-agentic,pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2225) instead of appending a brand-new entry for this PR (#2233). Git history confirms this entry was created by an earlier commit and last touched to point at #2225 — it is not new content introduced by this PR, it's a historical record being overwritten.Why this breaks CI. AGENTS.md states perf-changelog.yaml is append-only: "new entries MUST be appended to the END, never inserted in the middle or prepended," and entries must never be deleted or modified. This is not just a style convention — it's mechanically enforced:
utils/process_changelog.py'sget_added_lines()raisesValueError("Deletions are not allowed in perf-changelog.yaml...")on any non-whitespace deleted line. This diff deletes the old description line (- "B300: GPU-resident TP4 at conc [1,2,4,6,8,16]; ..."), which trips this check directly.utils/validate_perf_changelog.py'scompare_entries()allows an existing entry to change only itspr-link(a "pr-link-only correction"). Here thepr-linkis left atpull/2225while thedescriptionchanges — sowithout_pr_link(base) != without_pr_link(head)fires the"entry N changed; existing entries are immutable except for pr-link-only corrections"error.
Either check alone fails the changelog-processing step of the sweep pipeline (
process_changelog.pyruns in.github/workflows/run-sweep.yml), so this PR cannot merge as written.Secondary content problem. Even ignoring the append-only violation, the new description text doesn't match what
configs/nvidia-master.yamlactually changed in this same PR:- It says "GPU-resident TP4/TP8 at conc [1,2,4,6,8,12,16,20,24,28,32]" — but this PR removes the standalone GPU-resident TP8 entry; only
tp:4carries that conc-list now. There is no TP8 GPU-resident arm to attribute it to. - It says "DEP8 at conc [32,64,96,128,160,192,196,224,228]" — the actual new DEP8 entry (
tp:8, ep:8, dp-attn:true, kv-offloading:none) has conc-list[64,96,112,128,144,160,176,192,224]. The changelog value is essentially the old pre-PR DEP8 list[32,64,96,128,160,192,224,228]with a stray196spliced in — it reads as leftover/unedited text, not a description of this PR's change. - It says "DEP4 at conc [8,16,24,32,40,48,56,64,72]" — the actual DEP4 (SimpleCPU) list is
[32,40,48,56,64,72]. - It says "with both SimpleCPU and Mooncake" for DEP4 — but this PR deletes the DEP4 Mooncake search-space entry entirely; only SimpleCPU remains.
Step-by-step proof of the CI failure:
- Base entry (before this PR) at line 4794-4798 has description ending in
...DEP8 at conc [32,64,96,128,160,192,224,228]; with both SimpleCPU and Mooncake 0.3.11.post1."andpr-link: .../pull/2225. - This PR's diff changes only the description line (line 4797) — the
pr-linkline (4798) is untouched, stillpull/2225. process_changelog.pydiffs base vs head; it sees the old description line was removed (a deletion of non-whitespace content) → raisesValueError, failing the sweep's changelog-processing step.- Independently,
validate_perf_changelog.py compare_entries()matches base and head entries by position/config-keys, computeswithout_pr_link()for each, finds they differ (description changed) whilepr-linkis identical → raisesChangelogValidationError("entry N changed..."). - Either failure blocks the PR from passing CI.
How to fix. Leave the #2225 entry byte-for-byte untouched, and append a brand-new entry at the end of the file with
config-keys: dsv4-fp4-b300-vllm-agentic,pr-linkpointing at #2233, and a description whose conc-lists actually match this PR'sconfigs/nvidia-master.yamlchanges: TP4 GPU-resident[1,2,4,6,8,12,16,20,24,28,32], TP4 SimpleCPU[28,32,36,40], DEP4 SimpleCPU[32,40,48,56,64,72], and DEP8 (tp:8, ep:8, dp-attn:true, kv-offloading:none)[64,96,112,128,144,160,176,192,224], with no mention of Mooncake for DEP4 since that arm is removed. -
🟡
configs/nvidia-master.yaml:3237-3238— The comment '# DEP8 SimpleCPU' above the new DEP8 search-space entry is mislabeled: that entry setskv-offloading: nonewith nokv-offload-backend, making it GPU-resident, not SimpleCPU-offloaded. It should read something like# DEP8 GPU-residentto match the actual config and avoid confusing future edits.Extended reasoning...
In
configs/nvidia-master.yamlunderdsv4-fp4-b300-vllm-agentic, the new DEP8 search-space entry is:# DEP8 SimpleCPU - { tp: 8, ep: 8, dp-attn: true, kv-offloading: none, conc-list: [64, 96, 112, 128, 144, 160, 176, 192, 224], router: { name: vllm-router, version: "0.1.14" } }
This entry sets
kv-offloading: noneand has nokv-offload-backendfield at all. Looking atdsv4_fp4_b300_vllm.sh, theKV_OFFLOAD_BACKENDcase statement treats an empty/unset backend as therequire_agentic_kv_offload_noneGPU-resident path — theSimpleCPUOffloadConnectorbranch is only reached whenkv-offload-backendnamesvllm-simpleexplicitly. So this arm is GPU-resident, not "SimpleCPU"-offloaded, yet the comment directly above it says# DEP8 SimpleCPU.This is corroborated by three independent pieces of evidence in the same PR:
- The script's own updated top-of-file comment:
# TP4, TP8, and DEP8 (TP8 + DP-attention) are GPU-resident (KV_OFFLOADING=none). - The new
IS_DEP8branch in the script, which is explicitly the GPU-resident, high-concurrency arm (--max-num-batched-tokens 16384,--long-prefill-token-threshold 4096,--gpu-memory-utilization 0.92) — none of which pertains to SimpleCPU offload tuning. - The
perf-changelog.yamlentry, which groups DEP8 together with the GPU-resident TP4/TP8 arms ("GPU-resident TP4/TP8 ... and DEP8 at conc [...] (max-num-batched-tokens 16384, long-prefill-token-threshold 4096, gpu-memory-utilization 0.92)"), separately from the SimpleCPU/Mooncake DEP4 arm.
The three sibling comments in the same search-space block (
# TP4 GPU-resident,# TP4 SimpleCPU,# DEP4 SimpleCPU) are all internally consistent with their entries'kv-offloading/kv-offload-backendfields, which makes the# DEP8 SimpleCPUlabel stand out as the one inconsistency. It's most likely a leftover from the old DEP8 arms this PR replaced, which did usekv-offloading: dramwithvllm-simple/mooncakebackends (see the removed lines in the diff) — the label wasn't updated when the entry'skv-offloadingvalue was changed tonone.Step-by-step proof:
- Read the entry:
kv-offloading: none, nokv-offload-backendkey present. - In the script,
KV_OFFLOAD_BACKENDis derived fromkv-offload-backend.name(absent here → empty string). - The
case "$KV_OFFLOAD_BACKEND" in ""branch callsrequire_agentic_kv_offload_none, i.e. the GPU-resident path — not thevllm-simplebranch that builds theSimpleCPUOffloadConnectorconfig. - Therefore this arm never touches
SimpleCPUOffloadConnector; "SimpleCPU" in the comment is factually wrong.
Impact: Since YAML comments aren't parsed, there is no runtime effect from this PR as-is — the sweep behavior is entirely driven by the field values, which are correct and consistent with the script and changelog. The risk is purely documentation-fidelity: a future editor trusting the "SimpleCPU" label could mistakenly add a
kv-offload-backendfield to this entry, which would break therequire_agentic_kv_offload_noneassumption baked into the script's case statement forkv-offloading: none.Fix: Simply rename the comment to
# DEP8 GPU-residentto match the actual field values and be consistent with the sibling# TP4 GPU-residentlabel. - The script's own updated top-of-file comment:
| # TP4, TP8, and DEP8 (TP8 + DP-attention) are GPU-resident (KV_OFFLOADING=none). | ||
| # DEP4 uses KV_OFFLOADING=dram with KV_OFFLOAD_BACKEND=vllm-simple or mooncake. |
There was a problem hiding this comment.
🟡 The header comment (lines 15-16) says TP4 is always GPU-resident (KV_OFFLOADING=none), but this same PR adds a new TP4 SimpleCPU dram-offload arm in nvidia-master.yaml plus a dedicated SIMPLE_LAZY_OFFLOAD code path for exactly that case. The comment should mention that plain TP4 can also run as a dram/SimpleCPU-offload arm; this is doc-only and has no runtime effect since the actual offload mode is driven by the per-arm kv-offloading config field, not the header prose.
Extended reasoning...
What the bug is
The updated header comment in dsv4_fp4_b300_vllm.sh (lines 15-16) reads:
# TP4, TP8, and DEP8 (TP8 + DP-attention) are GPU-resident (KV_OFFLOADING=none).
# DEP4 uses KV_OFFLOADING=dram with KV_OFFLOAD_BACKEND=vllm-simple or mooncake.
This asserts that plain TP4 (non-DP-attention) is only ever run GPU-resident. That statement is no longer accurate as of this same PR.
The contradicting code path this PR adds
This PR simultaneously adds a new # TP4 SimpleCPU search-space entry to configs/nvidia-master.yaml:
- { tp: 4, kv-offloading: dram, kv-offload-backend: { name: vllm-simple, version: "904e4ec" }, conc-list: [28, 32, 36, 40] }That is plain TP4 (dp-attn unset/false) with kv-offloading: dram. Even more tellingly, the same PR adds a dedicated shell code path for exactly this arm:
# The plain-TP (non-DP-attention) offload ladder uses lazy offload;
# DEP keeps eager offload for cross-rank block-hash stability.
SIMPLE_LAZY_OFFLOAD=false
if [ "$DP_ATTENTION" != "true" ]; then
SIMPLE_LAZY_OFFLOAD=true
fiSIMPLE_LAZY_OFFLOAD is set to true precisely when DP_ATTENTION != true — i.e. for the plain-TP4-dram case introduced by the new yaml arm. The perf-changelog entry even documents it explicitly: "TP4 SimpleCPU lazy-offload at conc [28,32,36,40]".
So the script now internally contradicts itself: the header claims TP4 is GPU-resident-only, while the body implements (and the changelog documents) a plain-TP dram-offload ladder. The stale mid-file comment near the OFFLOAD_ARGS case statement ("TP4 remains GPU-resident") has the same issue.
Why existing code does not prevent this
Nothing enforces the comment against the actual config — KV_OFFLOADING/KV_OFFLOAD_BACKEND are supplied per-arm at invocation time from nvidia-master.yaml, and the header is just prose. There is no lint or test tying documentation to the search-space definitions, so the two can silently drift, which is exactly what happened here.
Step-by-step proof
- Sweep launcher reads
configs/nvidia-master.yaml, iterates thedsv4-fp4-b300-vllm-agenticsearch space. - It hits the new
# TP4 SimpleCPUentry:tp: 4,kv-offloading: dram,kv-offload-backend: vllm-simple, nodp-attn. - The launcher invokes
dsv4_fp4_b300_vllm.shwithTP=4,KV_OFFLOADING=dram,KV_OFFLOAD_BACKEND=vllm-simple,DP_ATTENTIONunset/false. - Inside the script,
DP_ATTENTION != "true", soSIMPLE_LAZY_OFFLOAD=trueand the vllm-simple offload branch of thecasestatement runs, configuringSimpleCPUOffloadConnectorwithlazy_offload: true— a genuine CPU-offload run, not GPU-resident. - Yet a reader relying on the header comment at lines 15-16 (or the mid-file "TP4 remains GPU-resident" note) would conclude this configuration is impossible/mislabeled, since the header states TP4 is GPU-resident only.
Suggested fix
Update the header (and the mid-file comment) to something like: "TP8 and DEP8 are GPU-resident (KV_OFFLOADING=none). TP4 and DEP4 each have both a GPU-resident and a dram/SimpleCPU-offload arm; DEP4 additionally supports mooncake." This keeps the documentation in sync with the search-space entries and the new SIMPLE_LAZY_OFFLOAD code path added by this PR.
Impact
Purely cosmetic — the comment does not drive any runtime behavior, since offload mode is controlled by the KV_OFFLOADING/KV_OFFLOAD_BACKEND env vars set per-arm by the yaml config, not by the prose. No functional bug, just a doc inaccuracy worth a follow-up fix. Marking as nit, consistent with all verifier assessments.
4 participants
No description provided.